Code Review

Overview

Adds a screenly screen status subcommand that fetches all screens via v4/screens?select=id,status,in_sync and prints a 1-row table with total / online / offline / out-of-sync counts. Supports --json.

Correctness

• offline = total - online overcounts on non-binary statuses (src/commands/screen.rs:80). The implementation filters explicitly for "Online", but assumes everything else is offline. If the API ever returns "Unknown", "Connecting", null, or a typo'd value, those silently roll into "offline." Safer: filter explicitly for "Offline" too, or count total - online_count - other_count and surface the discrepancy. At minimum the assumption deserves a comment.

• out_of_sync includes offline screens (src/commands/screen.rs:75-78). A screen that is Offline will typically have in_sync: false simply because it can't report. So the "out of sync" count is dominated by offline screens, which is probably not what the user wants from a status summary. Consider scoping to online screens (status == "Online" && in_sync != true), or renaming the column to make the semantics explicit.

• Silent fallback to empty on bad response shape (src/commands/screen.rs:71): data.as_array().map(|a| a.as_slice()).unwrap_or(&[]) masks an API contract violation as "0 screens." Other commands in this file propagate via ? and let the error surface. Prefer returning CommandError::MissingField (or similar) if the response is not an array.

• No pagination handling. The v4 endpoint may paginate large result sets, in which case the totals would be silently wrong for accounts with many screens. Worth verifying whether the API paginates this endpoint and either documenting or handling it.

Conventions / reuse

• Bypasses Formatter + handle_command_execution_result (src/cli.rs:658-672). Every other Screen command uses the shared helper. Implementing Formatter on ScreensStatus would let this branch collapse to one line and inherit the project's standard error formatting (including the dedicated Authentication error message that this branch loses):

  ScreenCommands::Status { json } => {
      handle_command_execution_result(screen_command.status(), json);
  }

• Redundant ScreenCommand construction (src/cli.rs:659). One is already built at src/cli.rs:640; the new screen_status_command shadows it for no reason. Drop the local and reuse screen_command.

• json: Option<bool> comparison uses json == &Some(true) whereas every other branch uses json.unwrap_or(false) via the shared helper — another reason to route through the helper.

Tests

• Three unit tests cover the count logic (mixed, all-online, empty) — good coverage on the math.
• No test for ScreensStatus::format. Other commands have a HumanReadable formatting test (test_format_screen_when_human_readable_output_is_set_...). Worth adding one for both Json and HumanReadable to lock in the table shape and JSON keys (public surface).
• The mock matches on query_param("select", "id,status,in_sync") — ties tests to the exact field list, which is fine but means any future field addition needs the test updated.

Summary

Functionally close, but the offline = total − online and out_of_sync includes offline semantics are the two things I'd most want clarified before merging — they directly affect what the numbers mean. The structural cleanup (implement Formatter, route through handle_command_execution_result, drop the duplicate ScreenCommand) would bring this in line with the rest of the file and is straightforward.